Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: env variables for ACTION_HISTORY limits #1261

Closed
wants to merge 3 commits into from

Conversation

vviers
Copy link
Collaborator

@vviers vviers commented Oct 14, 2024

Context

Partial fix of #1121

As a self-hoster, I want to be able to parameterize ACTION_HISTORY max sizes (both as rows and as disk space)

Proposed solution

Introduce 2 env variables to make those limits parametrizable.

Related issues

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here
  • 🙋 no, because I need help

Screenshots / Screencasts

const ACTION_HISTORY_MAX_BYTES = 1000 * 1000 * 1000; // 1 GB.
const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1250 rows / 1.25 GB.
const ACTION_HISTORY_MAX_ROWS = process.env.ACTION_HISTORY_MAX_ROWS || 1000;
const ACTION_HISTORY_MAX_BYTES = process.env.ACTION_HISTORY_MAX_BYTES || 1000 * 1000 * 1000; // 1 GB.
Copy link
Collaborator

@fflorent fflorent Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may rather consider using the section util, like this (and also for the line above):

const ACTION_HISTORY_MAX_BYTES = section.flag('actionHistoryMaxBytes').requireInt({
      envVar: 'ACTION_HISTORY_MAX_BYTES',
      defaultValue: 1000 * 1000 * 1000,
    });

This way, you are sure about the type, and you have default value.
Also if the passed string is not a number, it raises an error for you and the server crashes immediately (instead of having erratic behavior).

You may take a look at OIDCConfig for an example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I used parseInt instead, does that still work ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I wouldn't know in which "section" to put it ? Should I create a new one ? I'm not too familiar with this object.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I used parseInt instead, does that still work ?

To work, you should check that the result of that function does not return NaN and throw otherwise.

Also it feels more future-proof (in case Grist stores the configuration in a file or in the database).

Also, I wouldn't know in which "section" to put it ? Should I create a new one ?

I would say so.

Also I would control the minimum value (and maybe the maximum). There is no such thing in the current implementation of readInt / requireInt, I would suggest to do that. If you need help on it, or need me to make this addition, please ask.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the env variable names should be prefixed with GRIST_: GRIST_ACTION_HISTORY_MAX_ROWS and GRIST_ACTION_HISTORY_MAX_BYTES.

It is a good practice, in order to avoid name collision with other app that would run in the same shell.

const ACTION_HISTORY_MAX_ROWS = 1000;
const ACTION_HISTORY_MAX_BYTES = 1000 * 1000 * 1000; // 1 GB.
const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1250 rows / 1.25 GB.
const ACTION_HISTORY_MAX_ROWS = parseInt(process.env.ACTION_HISTORY_MAX_ROWS, 10) || 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the Typescript to do what you want is

Suggested change
const ACTION_HISTORY_MAX_ROWS = parseInt(process.env.ACTION_HISTORY_MAX_ROWS, 10) || 1000;
const ACTION_HISTORY_MAX_ROWS = Number(process.env.ACTION_HISTORY_MAX_ROWS) || 1000;

If what you set in env is NaN, let say "test" it will fallback to default value 1000.

Copy link
Collaborator

@hexaltation hexaltation Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but @fflorent suggestion is better.

const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1250 rows / 1.25 GB.
const ACTION_HISTORY_MAX_ROWS = parseInt(process.env.ACTION_HISTORY_MAX_ROWS, 10) || 1000;
const ACTION_HISTORY_MAX_BYTES = parseInt(process.env.ACTION_HISTORY_MAX_BYTES, 10) || 1000 * 1000 * 1000; // 1 GB.
const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1.25 times the above limits.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we want to give access to ACTION_HISTORY_GRACE_FACTOR and ACTION_HISTORY_CHECK_PERIOD in env too ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like:

  • not that simple to explain
  • can be dangerous if the value is not controlled

Also YAGNI, so I would leave it as is and let a user either report the need or make a contribution if they really need to

const ACTION_HISTORY_MAX_BYTES = 1000 * 1000 * 1000; // 1 GB.
const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1250 rows / 1.25 GB.
const ACTION_HISTORY_MAX_ROWS = parseInt(process.env.ACTION_HISTORY_MAX_ROWS, 10) || 1000;
const ACTION_HISTORY_MAX_BYTES = parseInt(process.env.ACTION_HISTORY_MAX_BYTES, 10) || 1000 * 1000 * 1000; // 1 GB.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here

Suggested change
const ACTION_HISTORY_MAX_BYTES = parseInt(process.env.ACTION_HISTORY_MAX_BYTES, 10) || 1000 * 1000 * 1000; // 1 GB.
const ACTION_HISTORY_MAX_BYTES = Number(process.env.ACTION_HISTORY_MAX_BYTES) || 1000 * 1000 * 1000; // 1 GB.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both should work here, I feel like this should make no difference.

const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1250 rows / 1.25 GB.
const ACTION_HISTORY_MAX_ROWS = parseInt(process.env.ACTION_HISTORY_MAX_ROWS, 10) || 1000;
const ACTION_HISTORY_MAX_BYTES = parseInt(process.env.ACTION_HISTORY_MAX_BYTES, 10) || 1000 * 1000 * 1000; // 1 GB.
const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1.25 times the above limits.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like:

  • not that simple to explain
  • can be dangerous if the value is not controlled

Also YAGNI, so I would leave it as is and let a user either report the need or make a contribution if they really need to

const ACTION_HISTORY_MAX_BYTES = 1000 * 1000 * 1000; // 1 GB.
const ACTION_HISTORY_GRACE_FACTOR = 1.25; // allow growth to 1250 rows / 1.25 GB.
const ACTION_HISTORY_MAX_ROWS = process.env.ACTION_HISTORY_MAX_ROWS || 1000;
const ACTION_HISTORY_MAX_BYTES = process.env.ACTION_HISTORY_MAX_BYTES || 1000 * 1000 * 1000; // 1 GB.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I used parseInt instead, does that still work ?

To work, you should check that the result of that function does not return NaN and throw otherwise.

Also it feels more future-proof (in case Grist stores the configuration in a file or in the database).

Also, I wouldn't know in which "section" to put it ? Should I create a new one ?

I would say so.

Also I would control the minimum value (and maybe the maximum). There is no such thing in the current implementation of readInt / requireInt, I would suggest to do that. If you need help on it, or need me to make this addition, please ask.

@fflorent fflorent added the anct label Oct 15, 2024
@fflorent fflorent self-assigned this Oct 15, 2024
@fflorent
Copy link
Collaborator

I resume the work on this PR here: #1262

@fflorent fflorent closed this Oct 15, 2024
@fflorent fflorent removed the anct label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants